Expose all settings in ydata-profiling ProfileReport #2921
Conversation
Signed-off-by: Eduardo Apolinario <[email protected]>
eapolinario
requested review from
wild-endeavor,
kumare3,
pingsutw,
cosmicBboy,
samhita-alla,
thomasjpfan and
Future-Outlier
as code owners
| @@ -53,12 +53,21 @@ class FrameProfilingRenderer: | |||
| Generate a ProfileReport based on a pandas DataFrame | |||
| """ | |||
|
|
|||
| def __init__(self, title: str = "Pandas Profiling Report"): | |||
| def __init__(self, title: Optional[str] = None): | |||
There was a problem hiding this comment.
API wise, there are now two places to set kwargs for ydata_profiling.ProfileReport, in __init__ (for only title) and in to_html.
By looking at the Renderable protocol, it looks like the design was to have configuration passed in __init__, because the to_html to restricted to one input:
https://github.com/flyteorg/flytekit/blob/master/flytekit/deck/renderer.py#L17-L23
There was a problem hiding this comment.
Yeah, I saw that, but I don't think we quite adhering to the protocol everywhere, for example the pandera renderer.
This is an indication that all invocations of to_html happen in user-code, so the protocol is "just" a convention? Can we make it more strict (and if so, I think we should add kwargs to the protocol).
|
/review |
Code Review Agent Run #3d8b84Actionable Suggestions - 1
Additional Suggestions - 1
Review Details
AI Code Review powered by |
Changelist by BitoThis pull request implements the following key changes.
|
| ("Custom Title", {"minimal": True}, "Custom Title"), | ||
| ("Custom Title", { "minimal": True}, "Custom Title"), |
There was a problem hiding this comment.
Consider removing the duplicate test case ("Custom Title", { "minimal": True}, "Custom Title") as it is identical to the test case on line 44. A similar issue was also found in plugins/flytekit-deck-standard/tests/test_renderer.py (line 44-45).
Code suggestion
Check the AI-generated fix before applying
| ("Custom Title", {"minimal": True}, "Custom Title"), | |
| ("Custom Title", { "minimal": True}, "Custom Title"), | |
| ("Custom Title", {"minimal": True}, "Custom Title"), |
Code Review Run #3d8b84
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Why are the changes needed?
ydata-profiling
ProfileReporthas a rich set of settings that should be exposed in flyte decks.What changes were proposed in this pull request?
Expose
kwargspassed directly to the constructor ofProfileReport.I set the minimal version of
ydata-profilingto2.4.0to support minimal-mode.How was this patch tested?
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Related to flyteorg/flyte#5993
Docs link
Summary by Bito
This PR enhances ydata-profiling integration in flytekit-deck-standard by exposing all ProfileReport constructor settings. The FrameProfilingRenderer now supports custom configuration via kwargs while maintaining backward compatibility. The minimum ydata-profiling version is set to 2.4.0 to support features like minimal-mode.Unit tests added: True
Estimated effort to review (1-5, lower is better): 2